fix(pcd): sizeSum calculation + rgb parsing #3101
Conversation
jclaessens97
changed the title
jclaessens97
changed the title
|
@jclaessens97 Thanks for the contribution.
|
| color.push(dataview.getUint8(row + offset.rgb + 0)); | ||
| color.push(dataview.getUint8(row + offset.rgb + 1)); | ||
| color.push(dataview.getUint8(row + offset.rgb + 2)); | ||
| color.push(dataview.getUint8(row + offset.rgb + 2) / 255.0); |
There was a problem hiding this comment.
Agree that using normalized colors is better. However this is a breaking change and should probably be mentioned in docs/upgrade-guide.md.
I am on the fence if it is worth it, but we could avoid breaking if we offered a pcd.normalizeColors loader option on the PCDLoader.
I have some concerns that we have code that creates a typed array from this array and if that typed array is Uint8Array it might fail (all values will be 0 or 1). Not sure if it is easy for you to find, I will try to dig more into this when I find some time.
There was a problem hiding this comment.
Hi, I'm perfectly fine by adding a normalizedColors option to avoid the breaking change!
Not sure what you mean by the last section about the Uint8Array. I think this is not applicable when we introduce a new array, right?
About the linting, I think I already ran the linter before pushing, but I can check again.
I wasn't able to run the tests so far. I had 2 failing thests from scratch, something to do with the paths I think, so if you have any pointers here, I'm happy to add some more tests with smaller examples!
There was a problem hiding this comment.
Not sure what you mean by the last section about the Uint8Array. I think this is not applicable when we introduce a new array, right?
There is code in loaders.gl that can convert parsed data to e.g. binary representations, in which case it might copy this JS array into a Uint8Array.
I'd have to dig into exactly what is implemented for meshes in terms of conversion right now to verify if it is an issue at this time.
Description
rowSize calculation
I tried to drop-in replace our
PCDLoaderwith the@loaders.glversion, but I noticed a difference in therowSizecalculation forbinaryPCD's. When comparing to our version which was based on the threejs example, I noticed a difference in the calculation. Replacing it with that implementation makes this loader work for me as well.RGB calculation
In the original three implementation, color need to be normalized between 0 and 1 by dividing by
255.0. Also the order of RGB needed to be reversed (BGR). This didn't happen for thebinaryversion and the order was also wrong for thebinary_compressedversion.I'll provide 2 binary PCD's that were broken in the original implementation, that are now fixed.
pcd.zip